Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for creating queries via initialized JpqlDsl object #642

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

jbl428
Copy link
Contributor

@jbl428 jbl428 commented Feb 18, 2024

Motivation

Modifications

  • Add overloading methods that takes jdsl object
  • Update documentation

영문문서는 한글 내용을 먼저 확인받고 진행해보겠습니다!

Commit Convention Rule

Commit type Description
feat New Feature
fix Fix bug
docs Documentation only changed
ci Change CI configuration
refactor Not a bug fix or add feature, just refactoring code
test Add Test case or fix wrong test case
style Only change the code style(ex. white-space, formatting)
chore It refers to minor tasks such as library version upgrade, typo correction, etc.
  • If you want to add some more commit type please describe it on the Pull Request

Result

  • There will be a new entry point to use the JpqlDsl created and managed by the user.

Closes

@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a1599c2) 66.79% compared to head (be449ce) 67.05%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #642      +/-   ##
===========================================
+ Coverage    66.79%   67.05%   +0.26%     
===========================================
  Files          479      479              
  Lines         4824     4863      +39     
  Branches       277      277              
===========================================
+ Hits          3222     3261      +39     
  Misses        1545     1545              
  Partials        57       57              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jbl428 jbl428 force-pushed the feat/user-managed-jpqldsl branch from dd434b7 to 6f0bc37 Compare February 18, 2024 02:00
Copy link
Member

@shouwn shouwn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the help! I'll change it a bit for the docs.

@shouwn
Copy link
Member

shouwn commented Feb 19, 2024

Oh, I see you're writing English after the Korean review, so I'll leave a review for docs as well.

{% hint style="info" %}
`jpql()`이 DSL을 인식하기 위해서 `JpqlDsl.Constructor`를 companion object로 구현해야 합니다.
{% endhint %}
나만의 DSL을 `jpql()`에 전달하기 위한 두 가지 방법이 있습니다.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example의 코드가 커서 dot으로는 문맥을 유지하는 것이 쉽지 않아 보입니다. 아래 처럼 먼저 방법을 이야기 하고 header를 이용해 예시를 나누는 것은 어떨까요?

이렇게 만들어진 나만의 DSL을 `jpql()`에 전달하기 위한 두 가지 방법이 있습니다. 첫 번째는 DSL 객체를 생성하는 `JpqlDsl.Constructor`를 companion object로 구현하는 것이고, 두 번째는 DSL 인스턴스를 생성하는 것입니다.

### JpqlDsl.Constructor
// description
// example

### Jpql Instance
// description
// example


val encryptor = Encryptor()
val instance = MyJpql(encryptor)
val query = jpql(instance) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jpql 객체 생성과 query의 생성 문맥을 분리하기 위해 다음과 같이 개행이 있으면 좋을 것 같아요.

val encryptor = Encryptor()
val instance = MyJpql(encryptor)

val query = jpql(instance) {
// ... query
}

Comment on lines 51 to 56
fun myFunction(value: String): Expression<String> {
val encrypted = encryptor.encrypt(value)
return function(String::class, "myFunction", listOf(value(encrypted)))
}
Copy link
Member

@shouwn shouwn Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예시로는 encryptor라는 특정 기능을 소개하고 있기 때문에 다음과 같이 encryptedValue 라는 메소드로 이름 지으면 어떨까요?

    fun encryptedValue(value: String): Expression<String> {
        val encrypted = encryptor.encrypt(value)
        return value(encrypted)
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제안해주신 내용으로 수정해봤는데 조금 더 실용적인 예제면 좋을거 같아서 암호화된 컬럼을 비교하는 코드로 수정해봤습니다!

@cj848
Copy link
Collaborator

cj848 commented Feb 20, 2024

리뷰 내용이 수정되면 리뷰를 추가 진행하겠습니다.

@jbl428 jbl428 force-pushed the feat/user-managed-jpqldsl branch from 6f0bc37 to 0692aba Compare February 21, 2024 11:12
@jbl428 jbl428 force-pushed the feat/user-managed-jpqldsl branch from 0692aba to 24bf3e8 Compare February 21, 2024 11:19
@@ -39,6 +42,35 @@ val query = jpql(MyJpql) {
}
```

### Jpql Instance

With this method, you can reuse a single instance for query creation and utilize dependency injection.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of way in the introduction and method in the details doesn't seem to fit together.

How about changing method to way?

Suggested change
With this method, you can reuse a single instance for query creation and utilize dependency injection.
With this way, you can reuse a single instance for query creation and utilize dependency injection.


fun Path<String>.equalToEncrypted(value: String): Predicate {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about removing a newline to make it a little shorter?

@jbl428 jbl428 force-pushed the feat/user-managed-jpqldsl branch from 24bf3e8 to be449ce Compare February 21, 2024 11:37
@shouwn
Copy link
Member

shouwn commented Feb 21, 2024

Thanks for your help!

@shouwn shouwn merged commit 8c5e93d into line:develop Feb 21, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants